Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SCHEMA] Add new MEG files and fix entity tables #898

Merged
merged 10 commits into from
Oct 18, 2021

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Oct 14, 2021

Closes #810.

NOTE: This adds in support for objects/dictionaries within datatype rules for entities, which may impact code using the schema. Tagging @bids-standard/schema-users in case this might be a problem for any of you.

Changes proposed:

  • Add new schema entries for crosstalk and calibration MEG files.
  • Incorporate allowed values (enums) into filename patterns.
  • Fix bugs in entity tables introduced by moi in [SCHEMA] Consolidate schema files by term type #883.
  • Allow multiple lines of the same datatypes/suffixes if the entity patterns differ.

@tsalo
Copy link
Member Author

tsalo commented Oct 14, 2021

On the one hand, the filename templates look awesome:

sub-<label>/
    [ses-<label>/]
        meg/
            sub-<label>[_ses-<label>]_task-<label>[_acq-<label>][_run-<index>][_proc-<label>][_split-<index>]_meg.<extension>
            sub-<label>[_ses-<label>]_task-<label>[_acq-<label>][_run-<index>][_proc-<label>][_split-<index>]_meg.json
            sub-<label>[_ses-<label>]_acq-<calibration>_meg.dat
            sub-<label>[_ses-<label>]_acq-<crosstalk>_meg.fif
            sub-<label>[_ses-<label>][_task-<label>][_acq-<label>][_space-<label>]_markers.mrk
            sub-<label>[_ses-<label>][_task-<label>][_acq-<label>][_space-<label>]_markers.sqd
            sub-<label>[_ses-<label>]_task-<label>[_acq-<label>][_run-<index>]_events.json
            sub-<label>[_ses-<label>]_task-<label>[_acq-<label>][_run-<index>]_events.tsv

On the other hand, the entity tables look terrible:

image

"also meg" was the best I came up with 😞, and I just realized that the way I wrote the code, it can only handle two rows with the same datatype/suffixes, so... thoughts?

@tsalo tsalo added the schema Issues related to the YAML schema representation of the specification. Patch version release. label Oct 14, 2021
@guiomar
Copy link
Collaborator

guiomar commented Oct 15, 2021

Can we name these files meg (shielding) or something similar? This goes for the calibration and crosstalk, right?

@tsalo
Copy link
Member Author

tsalo commented Oct 15, 2021

@guiomar there's no mechanism to label the different groups of suffixes in the schema at the moment. We could add one, though I'm always hesitant to add new fields to the schema.

EDIT: Yes, the new row corresponds to both calibration and crosstalk files.

@tsalo
Copy link
Member Author

tsalo commented Oct 15, 2021

As a more flexible, but not very informative, alternative, I've made it able to handle any number of duplicates and have dropped the "also":

image

@sappelhoff
Copy link
Member

thanks! looks all good except the entity table, as you say :-)

As a more flexible, but not very informative, alternative, I've made it able to handle any number of duplicates and have dropped the "also"

Can we not make it MEG (calibration & crosstalk) ?

@Remi-Gau
Copy link
Collaborator

Can we not make it MEG (calibration & crosstalk) ?

That would be fine with me.

The rest looks good. :-D

@tsalo
Copy link
Member Author

tsalo commented Oct 18, 2021

Can we not make it MEG (calibration & crosstalk) ?

@sappelhoff @Remi-Gau I can't think of a straightforward way to do that with the current schema, so I would need to add a new field, like "name", for labeling whole groups of suffixes in schema/rules/datatypes/*.yaml.

@Remi-Gau
Copy link
Collaborator

OK I am ok keeping like this for now

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am also OK with the current state. I think merging this and opening an issue on how to modify the schema that the meg (meg) can become a meg (crosstalk & calibration) would be a good way to go forward.

Thanks @tsalo for your work here

@guiomar
Copy link
Collaborator

guiomar commented Oct 18, 2021

Hi! I think I missed when the calibration and crosstalk files were incorporated to the specs. I think they weren't on the first release. Why do they have the _meg suffix? Shouldn't it have been better to have something like shielding for a suffix to disentangle from actual meg data? These are only required for SSS in Elekta systems, right? Maybe some discussions related to that happened that I missed, sorry!

@sappelhoff
Copy link
Member

sappelhoff commented Oct 18, 2021

Maybe some discussions related to that happened that I missed, sorry!

it happened here @guiomar :-)

@guiomar
Copy link
Collaborator

guiomar commented Oct 18, 2021

Thanks @sappelhoff! And it's not that you didn't tag me!

@tsalo
Copy link
Member Author

tsalo commented Oct 18, 2021

Is everyone okay with me merging?

@tsalo tsalo merged commit b4dbd65 into bids-standard:master Oct 18, 2021
@tsalo tsalo deleted the meg-stuff branch October 18, 2021 21:10
@sappelhoff sappelhoff added the exclude-from-changelog This item will not feature in the automatically generated changelog label Aug 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exclude-from-changelog This item will not feature in the automatically generated changelog schema Issues related to the YAML schema representation of the specification. Patch version release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SCHEMA] MEG files not yet covered by the schema
4 participants